Merged
Conversation
Allows launching specific shards directly using a new `--shard` command-line argument. The launcher now tracks the last played date for shards and populates the Windows Taskbar Jump List with recently played entries for quick access. This change also updates several dependencies and adds license headers to modified files.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis pull request adds command-line argument parsing to the launcher application to enable shard selection via command-line parameters, introduces LastPlayed timestamp tracking for shards with Windows Jump List integration for quick access, enhances packet length verification in the network layer with diagnostic error reporting, and updates related package dependencies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Centralizes packet length validation logic into a reusable method and extends verification to incoming packets within the filtering pipeline. Includes a new hex dump utility to provide better diagnostic information when invalid packet lengths are encountered.
Reetus
force-pushed
the
verify-filtered-packet-length
branch
from
de3df24 to
4a14486
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ClassicAssist.Launcher/MainViewModel.cs (1)
657-669:⚠️ Potential issue | 🟠 Major
LastPlayedis not persisted for non-preset shards.Non-preset shards have
LastPlayeddeserialized on load (line 155), but it's not serialized here on save. This means user-added shards will lose their "recently played" timestamp after restart, breaking the Jump List ordering for these shards.Compare with preset serialization (lines 714-715) which does include
LastPlayed.🔧 Proposed fix - add LastPlayed to serialization
JObject shardObj = new JObject { { "Name", shard.Name }, { "Address", shard.Address }, { "Port", shard.Port }, { "HasStatusProtocol", shard.HasStatusProtocol }, - { "Encryption", shard.Encryption } + { "Encryption", shard.Encryption }, + { "LastPlayed", shard.LastPlayed } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ClassicAssist.Launcher/MainViewModel.cs` around lines 657 - 669, The loop that serializes non-preset shards (iterating over ShardEntry shard and building shardObj added to shardArray) omits the LastPlayed property, so LastPlayed should be added to shardObj like the preset serialization does; update the serialization block that creates JObject shardObj for each ShardEntry to include { "LastPlayed", shard.LastPlayed } (ensure the property name and value format match the preset branch) so user-added shards persist their LastPlayed timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ClassicAssist.Launcher/App.xaml.cs`:
- Around line 27-35: OnStartup only sets App.CurrentOptions inside the
successful parse callback
(Parser.Default.ParseArguments<CommandLineOptions>(e.Args).WithParsed(...)),
leaving CurrentOptions null on parse failure; change the logic in OnStartup so
that CurrentOptions is always initialized: call
Parser.Default.ParseArguments<CommandLineOptions>(e.Args) and in the WithParsed
set CurrentOptions to the parsed value, and in the WithNotParsed (or after
parse) assign a sensible default new CommandLineOptions() to CurrentOptions (or
fall back to defaults) so MainViewModel can safely access
App.CurrentOptions.Shard without null checks.
In `@ClassicAssist.Launcher/MainViewModel.cs`:
- Around line 203-219: The null check around setting SelectedShard is redundant
because shard is guaranteed non-null due to the early return; remove the if
(shard != null) block and directly assign SelectedShard = shard, then call
StartCommand.Execute(null). Update the code paths involving ShardEntry shard,
ShardManager.VisibleShards.FirstOrDefault(...), SelectedShard, and
StartCommand.Execute to reflect this simplified flow.
- Around line 562-575: The Jump List currently sorts played shards ascending
using OrderBy on LastPlayed so oldest appear first; change the sorting to
descending (e.g., use OrderByDescending on ShardManager.VisibleShards by
LastPlayed or call Reverse() on the IOrderedEnumerable) so playedShards yields
most recently played shards first before creating JumpTask entries (refer to
playedShards, ShardManager.VisibleShards, LastPlayed, OrderBy, JumpTask).
In `@ClassicAssist/Engine.cs`:
- Around line 751-758: The Sentry capture is currently sending the raw packet
payload (packet) which may contain PII; update the Sentry reporting in the
SentrySdk.CaptureMessage block(s) used by Engine.SendPacketToServer to stop
adding the raw packet and instead add packet metadata: packet id (derive from
packet header), Direction (PacketDirection.Outgoing/Incoming), actual Length,
Expected Length, and a redacted preview string (e.g., a short hex/utf8 preview
where sensitive regions are masked or truncated). Replace
scope.SetExtra("Packet", packet) with scope.SetExtra("PacketId", packetId) and
scope.SetExtra("RedactedPreview", redactedPreview). Implement or call a small
helper (e.g., RedactPacketPreview(packet)) to produce the preview and use it in
both places mentioned (the shown CaptureMessage and the similar capture around
Engine.SendPacketToServer elsewhere) so no raw payload is sent to Sentry.
- Around line 865-881: VerifyPacketLengthCorrect currently ignores callers'
logical length and reads packet[1]/packet[2] blindly, causing false rejects and
potential index errors; change its signature to accept an int length (from
SendPacketToServer/SendPacketToClient callers) and use that length for
comparisons instead of packet.Length, and when _getPacketLength returns -1
ensure the provided length is at least 3 before reading packet[1]/packet[2]
(otherwise return false); update callers (SendPacketToServer/SendPacketToClient)
to pass their logical length through to VerifyPacketLengthCorrect.
In `@ClassicAssist/Misc/Utility.cs`:
- Around line 48-73: The FormatBuffer method can divide by zero when
bytesPerLine <= 0; validate and reject non-positive bytesPerLine at the start of
FormatBuffer (e.g., in the method body of FormatBuffer) by throwing an
ArgumentOutOfRangeException or defaulting to a sane positive value; ensure the
check runs before computing expectedLines = (bytesLength + bytesPerLine - 1) /
bytesPerLine and before any calculations that use bytesPerLine so the method
never attempts a division by zero.
---
Outside diff comments:
In `@ClassicAssist.Launcher/MainViewModel.cs`:
- Around line 657-669: The loop that serializes non-preset shards (iterating
over ShardEntry shard and building shardObj added to shardArray) omits the
LastPlayed property, so LastPlayed should be added to shardObj like the preset
serialization does; update the serialization block that creates JObject shardObj
for each ShardEntry to include { "LastPlayed", shard.LastPlayed } (ensure the
property name and value format match the preset branch) so user-added shards
persist their LastPlayed timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3c20d4a3-580d-478c-9273-8f4f2399fd77
📒 Files selected for processing (9)
ClassicAssist.Launcher/App.xaml.csClassicAssist.Launcher/ClassicAssist.Launcher.csprojClassicAssist.Launcher/CommandLineOptions.csClassicAssist.Launcher/MainViewModel.csClassicAssist.Launcher/ShardEntry.csClassicAssist.Launcher/Utility.csClassicAssist/Engine.csClassicAssist/Misc/Utility.csClassicAssist/UO/Network/IncomingPacketFilters.cs
Comment on lines
+562
to
+575
| IOrderedEnumerable<ShardEntry> playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderBy( e => e.LastPlayed ); | ||
|
|
||
| foreach ( ShardEntry shard in playedShards ) | ||
| { | ||
| jumpList.JumpItems.Add( new JumpTask | ||
| { | ||
| Title = shard.Name, | ||
| Arguments = $"--shard \"{shard.Name}\"", | ||
| ApplicationPath = fileName, | ||
| IconResourcePath = fileName, | ||
| WorkingDirectory = directory, | ||
| CustomCategory = "Shards" | ||
| } ); | ||
| } |
Contributor
There was a problem hiding this comment.
Jump List shows oldest played shards first instead of most recent.
OrderBy sorts in ascending order, so the oldest LastPlayed shards appear first in the Jump List. For a "recently played" feature, users would expect the most recently played shards at the top.
🔧 Proposed fix - use descending order
- IOrderedEnumerable<ShardEntry> playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderBy( e => e.LastPlayed );
+ IOrderedEnumerable<ShardEntry> playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderByDescending( e => e.LastPlayed );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ClassicAssist.Launcher/MainViewModel.cs` around lines 562 - 575, The Jump
List currently sorts played shards ascending using OrderBy on LastPlayed so
oldest appear first; change the sorting to descending (e.g., use
OrderByDescending on ShardManager.VisibleShards by LastPlayed or call Reverse()
on the IOrderedEnumerable) so playedShards yields most recently played shards
first before creating JumpTask entries (refer to playedShards,
ShardManager.VisibleShards, LastPlayed, OrderBy, JumpTask).
ClassicAssist/Engine.cs
Outdated
Comment on lines
+751
to
+758
| SentrySdk.CaptureMessage( $"Invalid packet length: {length} != {expectedLength}", scope => | ||
| { | ||
| SentrySdk.CaptureMessage( $"Invalid packet length: {length} != {expectedLength}", scope => | ||
| { | ||
| scope.SetExtra( "Packet", packet ); | ||
| scope.SetExtra( "Length", length ); | ||
| scope.SetExtra( "Direction", PacketDirection.Outgoing ); | ||
| scope.SetExtra( "Expected Length", expectedLength ); | ||
| } ); | ||
| scope.SetExtra( "Packet", packet ); | ||
| scope.SetExtra( "Length", length ); | ||
| scope.SetExtra( "Direction", PacketDirection.Outgoing ); | ||
| scope.SetExtra( "Expected Length", expectedLength ); | ||
| scope.SetExtra( "Callsite", "Engine.SendPacketToServer" ); | ||
| } ); |
Contributor
There was a problem hiding this comment.
Don't send raw packet payloads to Sentry.
These packets can still contain player speech, names, or shard-specific data. For this diagnostic path, packet id, direction, actual/expected length, and at most a redacted preview should be enough.
Also applies to: 795-802
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ClassicAssist/Engine.cs` around lines 751 - 758, The Sentry capture is
currently sending the raw packet payload (packet) which may contain PII; update
the Sentry reporting in the SentrySdk.CaptureMessage block(s) used by
Engine.SendPacketToServer to stop adding the raw packet and instead add packet
metadata: packet id (derive from packet header), Direction
(PacketDirection.Outgoing/Incoming), actual Length, Expected Length, and a
redacted preview string (e.g., a short hex/utf8 preview where sensitive regions
are masked or truncated). Replace scope.SetExtra("Packet", packet) with
scope.SetExtra("PacketId", packetId) and scope.SetExtra("RedactedPreview",
redactedPreview). Implement or call a small helper (e.g.,
RedactPacketPreview(packet)) to produce the preview and use it in both places
mentioned (the shown CaptureMessage and the similar capture around
Engine.SendPacketToServer elsewhere) so no raw payload is sent to Sentry.
ClassicAssist/Engine.cs
Outdated
Comment on lines
+865
to
+881
| public static bool VerifyPacketLengthCorrect( byte[] packet, out int expectedLength ) | ||
| { | ||
| expectedLength = packet.Length; | ||
|
|
||
| if ( _getPacketLength == null ) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| expectedLength = _getPacketLength( packet[0] ); | ||
|
|
||
| if ( expectedLength == -1 ) | ||
| { | ||
| expectedLength = ( packet[1] << 8 ) | packet[2]; | ||
| } | ||
|
|
||
| return packet.Length == expectedLength; |
Contributor
There was a problem hiding this comment.
Validate against the logical packet length and guard truncated headers.
SendPacketToServer/SendPacketToClient still pass a separate length, but this helper ignores it and compares packet.Length. That can reject a valid payload in a larger backing buffer, and packet[1]/packet[2] will throw on short variable-length packets — exactly the case this verifier is meant to report.
Suggested shape
-public static bool VerifyPacketLengthCorrect( byte[] packet, out int expectedLength )
+public static bool VerifyPacketLengthCorrect( byte[] packet, int length, out int expectedLength )
{
- expectedLength = packet.Length;
+ expectedLength = length;
+
+ if ( packet == null || length <= 0 || length > packet.Length )
+ {
+ return false;
+ }
if ( _getPacketLength == null )
{
return true;
}
expectedLength = _getPacketLength( packet[0] );
if ( expectedLength == -1 )
{
+ if ( length < 3 )
+ {
+ expectedLength = 3;
+ return false;
+ }
+
expectedLength = ( packet[1] << 8 ) | packet[2];
}
- return packet.Length == expectedLength;
+ return length == expectedLength;
}The callers should then pass their logical length through as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static bool VerifyPacketLengthCorrect( byte[] packet, out int expectedLength ) | |
| { | |
| expectedLength = packet.Length; | |
| if ( _getPacketLength == null ) | |
| { | |
| return true; | |
| } | |
| expectedLength = _getPacketLength( packet[0] ); | |
| if ( expectedLength == -1 ) | |
| { | |
| expectedLength = ( packet[1] << 8 ) | packet[2]; | |
| } | |
| return packet.Length == expectedLength; | |
| public static bool VerifyPacketLengthCorrect( byte[] packet, int length, out int expectedLength ) | |
| { | |
| expectedLength = length; | |
| if ( packet == null || length <= 0 || length > packet.Length ) | |
| { | |
| return false; | |
| } | |
| if ( _getPacketLength == null ) | |
| { | |
| return true; | |
| } | |
| expectedLength = _getPacketLength( packet[0] ); | |
| if ( expectedLength == -1 ) | |
| { | |
| if ( length < 3 ) | |
| { | |
| expectedLength = 3; | |
| return false; | |
| } | |
| expectedLength = ( packet[1] << 8 ) | packet[2]; | |
| } | |
| return length == expectedLength; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ClassicAssist/Engine.cs` around lines 865 - 881, VerifyPacketLengthCorrect
currently ignores callers' logical length and reads packet[1]/packet[2] blindly,
causing false rejects and potential index errors; change its signature to accept
an int length (from SendPacketToServer/SendPacketToClient callers) and use that
length for comparisons instead of packet.Length, and when _getPacketLength
returns -1 ensure the provided length is at least 3 before reading
packet[1]/packet[2] (otherwise return false); update callers
(SendPacketToServer/SendPacketToClient) to pass their logical length through to
VerifyPacketLengthCorrect.
Comment on lines
+48
to
+73
| public static void FormatBuffer( byte[] bytes, string title = null, int bytesPerLine = 16 ) | ||
| { | ||
| if ( bytes == null || bytes.Length == 0 ) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| int bytesLength = bytes.Length; | ||
|
|
||
| Console.WriteLine( !string.IsNullOrEmpty( title ) ? $"{title}, Packet: {bytes[0]:X}, Size: {bytes.Length}" : $"Packet: {bytes[0]:X}, Size: {bytes.Length}" ); | ||
|
|
||
| char[] HexChars = "0123456789ABCDEF".ToCharArray(); | ||
|
|
||
| const int firstHexColumn = 8 // 8 characters for the address | ||
| + 3; // 3 spaces | ||
|
|
||
| int firstCharColumn = firstHexColumn + bytesPerLine * 3 // - 2 digit for the hexadecimal value and 1 space | ||
| + ( bytesPerLine - 1 ) / 8 // - 1 extra space every 8 characters from the 9th | ||
| + 2; // 2 spaces | ||
|
|
||
| int lineLength = firstCharColumn + bytesPerLine // - characters to show the ascii value | ||
| + Environment.NewLine.Length; // Carriage return and line feed (should normally be 2) | ||
|
|
||
| char[] line = ( new string( ' ', lineLength - Environment.NewLine.Length ) + Environment.NewLine ).ToCharArray(); | ||
| int expectedLines = ( bytesLength + bytesPerLine - 1 ) / bytesPerLine; | ||
| StringBuilder result = new StringBuilder( expectedLines * lineLength ); |
Contributor
There was a problem hiding this comment.
Reject non-positive bytesPerLine.
expectedLines = ( bytesLength + bytesPerLine - 1 ) / bytesPerLine will throw when bytesPerLine <= 0, so this new public helper can fail before it prints anything. Guard the argument up-front.
Suggested fix
public static void FormatBuffer( byte[] bytes, string title = null, int bytesPerLine = 16 )
{
+ if ( bytesPerLine <= 0 )
+ {
+ throw new ArgumentOutOfRangeException( nameof( bytesPerLine ) );
+ }
+
if ( bytes == null || bytes.Length == 0 )
{
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ClassicAssist/Misc/Utility.cs` around lines 48 - 73, The FormatBuffer method
can divide by zero when bytesPerLine <= 0; validate and reject non-positive
bytesPerLine at the start of FormatBuffer (e.g., in the method body of
FormatBuffer) by throwing an ArgumentOutOfRangeException or defaulting to a sane
positive value; ensure the check runs before computing expectedLines =
(bytesLength + bytesPerLine - 1) / bytesPerLine and before any calculations that
use bytesPerLine so the method never attempts a division by zero.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Chores